Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Non-thread-safe subscriptions #160

Merged
merged 11 commits into from
Sep 20, 2024
Merged

Non-thread-safe subscriptions #160

merged 11 commits into from
Sep 20, 2024
+673 −27

Conversation

richardhuaaa
Copy link
Contributor

@richardhuaaa richardhuaaa commented Sep 12, 2024 •
edited
Loading

Introduces a basic initial version of client subscriptions. A worker maintains a global database subscription, and pushes updates to relevant client listeners.

There are many pieces coming in later PR's:

  • Thread safety for adding and reading listeners
  • Listener cleanup, esp when channels are full
  • Subscribing from a vector clock cursor (which also gives a way to recover from backpressure)
  • Logging and load/performance metrics
  • Bidirectional streaming

#125

@richardhuaaa Graphite App
Copy link
Contributor Author

richardhuaaa commented Sep 12, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @richardhuaaa and the rest of your teammates on Graphite Graphite

@richardhuaaa richardhuaaa mentioned this pull request Sep 12, 2024
Closed
@graphite-app Graphite App
Copy link

graphite-app bot commented Sep 12, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “Queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “Hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@richardhuaaa richardhuaaa force-pushed the 09-12-split_up_service_tests branch from 58c6ad6 to ee1edb4 Compare September 16, 2024 19:12
@richardhuaaa richardhuaaa force-pushed the 09-03-subscription_worker branch from f2c6b04 to 985e774 Compare September 16, 2024 19:37
@richardhuaaa richardhuaaa mentioned this pull request Sep 16, 2024
Merged
Base automatically changed from 09-12-split_up_service_tests to main September 16, 2024 20:41
@richardhuaaa richardhuaaa force-pushed the 09-03-subscription_worker branch from 985e774 to 1b33be3 Compare September 17, 2024 17:32
@richardhuaaa richardhuaaa changed the title Subscription worker Non-threadsafe subscription worker Sep 17, 2024
@richardhuaaa richardhuaaa changed the title Non-threadsafe subscription worker Non-thread safe subscription worker Sep 17, 2024
@richardhuaaa richardhuaaa changed the title Non-thread safe subscription worker Non-thread-safe subscriptions Sep 17, 2024
@richardhuaaa richardhuaaa marked this pull request as ready for review September 17, 2024 17:48
@richardhuaaa richardhuaaa requested a review from a team as a code owner September 17, 2024 17:48
neekolas
neekolas reviewed Sep 17, 2024
View reviewed changes
pkg/api/service.go Outdated Show resolved Hide resolved
neekolas
neekolas reviewed Sep 17, 2024
View reviewed changes
pkg/api/service.go Outdated Show resolved Hide resolved
mkysel
mkysel reviewed Sep 18, 2024
View reviewed changes
pkg/api/service.go Outdated Show resolved Hide resolved
@mkysel
Copy link
Collaborator

mkysel commented Sep 18, 2024

if a client installation subscribes multiple times, those subscriptions are all separate?

mkysel
mkysel reviewed Sep 18, 2024
View reviewed changes
pkg/api/subscribeWorker.go
select {
case <-s.ctx.Done():
return
case new_batch := <-s.dbSubscription:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit uncomfortable that we are passing a slice to a channel. In this simple use case its correct, but slices are mutable and not thread safe. So it makes me not trust this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you be more specific about the case you are worried about? In general I'm not sure if there's a hard-and-fast rule that all objects passed into a channel must be immutable (although it would be suspect if they were mutated while they were in the channel, which we don't do)

mkysel
mkysel reviewed Sep 18, 2024
View reviewed changes
pkg/api/subscribeWorker.go Outdated Show resolved Hide resolved
@mkysel
Copy link
Collaborator

mkysel commented Sep 18, 2024

I believe that you say this in the description of the PR, but I just want to confirm. Right now the client lastSeen is ignored. Implementing that is represented by Subscribing from a vector clock cursor?

@richardhuaaa
Copy link
Contributor Author

if a client installation subscribes multiple times, those subscriptions are all separate?

For each batchSubscribe() call, the client gets a separate stream. Within a single batchSubscribe() call, there may be multiple requests, all of which get aggregated into a single stream.

I believe that you say this in the description of the PR, but I just want to confirm. Right now the client lastSeen is ignored. Implementing that is represented by Subscribing from a vector clock cursor?

Yep, that's right. Picking up historical messages from before the stream was requested (without dropping messages or having duplicates) is a bit more complicated, so I'm splitting it off into a separate PR so as not to make this one harder to review.

@richardhuaaa richardhuaaa force-pushed the 09-03-subscription_worker branch from 0a6d801 to c7cfaca Compare September 18, 2024 18:43
@richardhuaaa richardhuaaa mentioned this pull request Sep 18, 2024
Merged
@richardhuaaa Graphite App
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @richardhuaaa and the rest of your teammates on Graphite Graphite

1 similar comment
@richardhuaaa Graphite App
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @richardhuaaa and the rest of your teammates on Graphite Graphite

@richardhuaaa richardhuaaa mentioned this pull request Sep 20, 2024
Merged
@richardhuaaa richardhuaaa merged commit 4caf7ad into main Sep 20, 2024
5 checks passed
@richardhuaaa richardhuaaa deleted the 09-03-subscription_worker branch September 20, 2024 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkysel mkysel mkysel left review comments

@neekolas neekolas neekolas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@richardhuaaa @mkysel @neekolas